-
-
Notifications
You must be signed in to change notification settings - Fork 249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WebSocket support for caliban-quick
#2150
Conversation
import zio.stream.ZStream | ||
|
||
trait StreamTransformer[-R, +E] { | ||
def transform[R1 <: R, E1 >: E](stream: ZStream[R1, E1, GraphQLWSOutput]): ZStream[R1, E1, GraphQLWSOutput] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m wondering if we could use a ZPipeline
for this instead of defining a new type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that would be ideal, although my only concern is that it's a user-facing breaking change.
Having said that, the migration should be very straightforward by simply using ZPipeline.fromFunction
so perhaps we should just do it and document the migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already breaking right? So as long as we are doing that can probably also move this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not bin-compatible, but other than a deprecation warning the previous changes were source compatible.
Actually I think we might be able to keep it source-compatible (at least for the deprecated trait). Let me give it a go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I overloaded the message
method in the deprecated WebSocketHooks
object to accept a StreamTransformer
, that way it should be source-compatible for users that don't manually create an instance of WebSocketHooks
case ChannelEvent.UserEventTriggered(HandshakeComplete) => | ||
out.runForeach(frame => ch.send(ChannelEvent.Read(frame))).forkScoped | ||
case ChannelEvent.Read(WebSocketFrame.Text(text)) => | ||
ZIO.suspend(queue.offer(readFromString[GraphQLWSInput](text))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulpdaniels @frekw, do you happen to know how should the server handle invalid messages? Based on this logic, we are going to raise an error if the frame cannot be decoded to GraphQLWSInput
, which AFAICT will close the stream.
Is this sound? Or should we be ignoring messages we can't decode?
I'll merge this one in before we start getting merge conflicts, but also so that we have a snapshot version to try out / test. |
Main changes:
Protocol
,WebSocketHooks
andStreamTransformer
into the core modulecaliban-quick
module and exposed methods to configure hooks & keep-alive durationZHttpAdapter
object, signalling that it will be removed in a future release